feat(instrumentation-express): Add support for Express v5 #2437
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2437 +/- ##
==========================================
- Coverage 90.86% 90.80% -0.07%
==========================================
Files 159 159
Lines 7849 7863 +14
Branches 1621 1627 +6
==========================================
+ Hits 7132 7140 +8
- Misses 717 723 +6
|
There was a problem hiding this comment.
"@types/express": "4.17.21"
There is now a v5 for the typing too
https://www.npmjs.com/package/@types/express
|
Thanks for working on this! I have this on my (admittedly long) list of things to review. Right now my biggest concern is ensuring this doesn't disrupt folks using express 4.x, especially with some of the breaking changes in v5. Express 5.x is still |
There was a problem hiding this comment.
As I am not familiar with the structure of this project's internals, I am not sure my review adds much value, but here it is as requested in Slack (cc @AbhiPrasad ) even if it is small. If you are comfortable with the fact that your test might not be durable then I would say it all looks good. Now that we know the removed Route api is part of someones tests we can try to remember to notify you if we do change it, just can't promise anything.
| moduleExports => { | ||
| const routerProto = moduleExports.Router as unknown as express.Router; | ||
| const isExpressV5 = 'Route' in moduleExports.Router; |
There was a problem hiding this comment.
I am not sure I like this kind of check. I have been thinking about this problem as well because another package I maintain had compatibility for the v5 router but we broke that in the final push to release. I don't particularly like the idea of importing the package.json at startup in express, but we dont limit you from doing it. I am not familiar with your testing setup so I don't know how feasible that is for you to do. My main concern with this is we could decide to add a Route constructor back (not that I know of any plans to do so) which would break this.
There was a problem hiding this comment.
Yeah agreed. I suspect it might be a better idea to target the two different versions separately 🤔
|
Sorry should have fully read the PR description before posting a review.
Is that this? If so, where do I see the assertions for this? Sorry it is a long PR and I just skimmed mostly. The new regular expression handling is here if you want to check. We moved it out of |
Here are the assertions which have been changed to pass. And here is the regex request handler that these are tested against. For v4, these paths resulted in |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }); | ||
|
|
||
| app.get('/post/:id', (req, res) => { | ||
| res.send(`Post id: ${req.params.id}`); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
| app.use('/double-slashes/', router); | ||
| router.get('/:id', (req, res) => { | ||
| setImmediate(() => { | ||
| res.status(200).end(req.params.id); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High test
|
I've improved the code to instrument differently depending on the express version but like for v4, the v5 instrumentation still relies on internals. One option would be to emit tracing events directly from express via |
This is the goal!! We will be putting together a group of folks interested in this and working on it in the express project. For anyone interested, there is an onging slack conversation you can join in the OpenJS slack: https://openjs-foundation.slack.com/archives/C02QB1731FH/p1729517144349069 |
Which problem is this PR solving?
Closes #2435
Short description of the changes
This PR adds support for express v5. The migration docs show few user facing API changes although the internals have changed quite a bit.
Express v5 has made Node v18 the minimum supported version although it looks like the tests only fail on v14 so far and >= v16 still pass.
The v5 tests are a copy of the v4 tests with some minor changes:
middleware - queryandmiddleware - expressInitspans are missing, no doubt due to reworking of the internals.*paths. Instead/.*/regex should be used.'/test/arr/:id,/\\/test\\/arr[0-9]*\\/required(path)?(\\/optionalPath)?\\/(lastParam)?/'but a couple of them instead contain/test,6,/test/. This is either a bug in express or caused by a change to their regex support mentioned in the migration guide. In these instances, the integration does not have access to the "correct" path in any other property.The v4 and v5 tests passed in isolation but when run though mocha together, many v4 tests failed with strange duplicate or missing spans. My best guess is that there's some incompatibility when running both express versions in the same process. To work around this I changed it to run these tests in isolated calls to mocha and then combine the nyc coverage in a posttest script.